Skip to content

Conversation

@jgsogo
Copy link
Contributor

@jgsogo jgsogo commented Oct 15, 2025

Changes are visible to end-users: yes

When creating a virtual environment, the activate script no longer modifies bash flags. These flags were added in #576 (v1.6.0) (note, they don't appear in the scripts that Python's venv module generates).

  • Searched for relevant documentation and updated as needed: yes (behaviour is not documented)
  • Breaking change (forces users to change their own code or config):no
  • Suggested release notes appear below: yes/no

Test plan

  • Manual testing; please provide instructions so we can reproduce:

Potentially, users could have some tests running the following commands. The new flags introduced in #576 broke this kind of tests:

#!/usr/bin/env bash
set -euo pipefail

# Create a virtualenvironment
bazel run //:venv

# Activate it (previous it was setting +e flag)
source .venv/bin/activate

# Some failing test -- users expects the script to end and fail here
false

# Some other command that doesn't fail
true

# !!!! Script returns 0 !!!!

@CLAassistant
Copy link

CLAassistant commented Oct 15, 2025

CLA assistant check
All committers have signed the CLA.

@aspect-workflows
Copy link

aspect-workflows bot commented Oct 15, 2025

Test

All tests were cache hits

45 tests (100.0%) were fully cached saving 1m 28s.

@jgsogo jgsogo marked this pull request as ready for review October 15, 2025 10:32
@arrdem arrdem changed the title venv - Consider not modifying bash flags in the activate script tweak(py_venv): Don't modifying shell flags in activate Oct 15, 2025
@arrdem arrdem self-requested a review October 15, 2025 15:52
Copy link
Collaborator

@arrdem arrdem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right we don't need to set these, think I added this to enable testing in earlier versions where there was sophisticated logic in activate we've moved away from that demanded aggressive error handling.

Thanks for the fix!

@arrdem arrdem enabled auto-merge (squash) October 15, 2025 15:53
@arrdem arrdem disabled auto-merge October 15, 2025 16:12
@arrdem
Copy link
Collaborator

arrdem commented Oct 15, 2025

Looks like GHA is having issues, tests passed I'm just gonna force merge.

@arrdem arrdem merged commit 84f5644 into aspect-build:main Oct 15, 2025
1 of 2 checks passed
@jgsogo jgsogo deleted the fix/keep-bash-flags branch October 15, 2025 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants